Skip to content

Add regression tests gating the mcp-go to go-sdk migration#5772

Merged
rdimitrov merged 5 commits into
feat/remove-mcp-gofrom
test/regression-mcp-go-migration
Jul 13, 2026
Merged

Add regression tests gating the mcp-go to go-sdk migration#5772
rdimitrov merged 5 commits into
feat/remove-mcp-gofrom
test/regression-mcp-go-migration

Conversation

@JAORMX

@JAORMX JAORMX commented Jul 9, 2026

Copy link
Copy Markdown
Collaborator

Summary

Adds a comprehensive regression test suite that pins the current mcpcompat shim behavior so the mcp-go → go-sdk migration (#5729) can be validated against it. The tests are written against toolhive-core/mcpcompat imports (not mark3labs/mcp-go) so they compile on this branch and survive the import swap.

Closes #5742.

Large PR Justification

This PR is ~2000 lines, almost entirely test code (18 files changed, only 4 contain production changes totaling ~35 lines). Per the repo's Contributing Guidelines and CLAUDE.md, test-only changes are exempt from the 400-line/10-file limit.

The tests cannot be meaningfully split because they form a single coherent regression gate for issue #5742's 14-group test matrix — they must land together so the migration PR (#5729) can be validated against the complete set. Splitting into per-group PRs would delay the migration gate and create partial coverage in the meantime.

The 4 production changes (bridge list_changed re-fetch ~12 lines, HeartbeatInterval config ~20 lines, WithSessionTTL helper ~13 lines, derive/serve field threading ~3 lines) are minimal and directly enable the tests.

What's tested

# Group File Status
1-2 Stdio bridge: progress/logging forwarding pkg/transport/bridge_test.go Skipped — shim drops progress/logging notifications (#5768)
3 Stdio bridge: tools/list_changed re-sync pkg/transport/bridge_test.go Pass (with bridge fix)
4 Stdio bridge: resources/prompts list_changed pkg/transport/bridge_test.go Covered by test 3's re-sync path
5 Session: terminated session rejected pkg/vmcp/server/session_lifecycle_regression_test.go Pass
6 Session: DELETE eviction same Pass
7 Session: identity binding same Pass
8 Projection: two sessions, different authz test/integration/vmcp/per_session_projection_regression_test.go Pass
9 Projection: filtered tool uncallable same Pass
10 Projection: fixed at initialize same Pass
11 SSE keep-alive pkg/vmcp/server/sse_keepalive_regression_test.go Pass (with HeartbeatInterval config)
12 Pagination >1000 tools test/integration/vmcp/pagination_regression_test.go Skipped — known gap (#5771)
13 Host-header rewrite pkg/transport/proxy/transparent/host_header_regression_test.go Pass
14 401 → ErrAuthenticationFailed pkg/vmcp/client/auth_error_mapping_regression_test.go Pass
15 403-on-initialize → legacy SSE same Pass
16 Capabilities advertised (CRITICAL) pkg/vmcp/server/capability_regression_test.go Pinned broken behavior — only {logging:{}} advertised (#5769)
17-18 Concurrent context isolation pkg/vmcp/server/context_isolation_regression_test.go Pass
22 Tool schema fidelity pkg/vmcp/server/schema_fidelity_regression_test.go Pinned broken behavior — schemas without type:object are normalized to {"type":"object"} (#5770)
23 Tool error "401" not auth failure pkg/vmcp/client/auth_error_mapping_regression_test.go Pass
24 403-on-init sentinel matching same Pass
25 Sliding session TTL test/integration/vmcp/sliding_ttl_regression_test.go Pass

Codebase changes

  • Bridge fix (pkg/transport/bridge.go): Re-fetch tools/resources/prompts when a *_list_changed notification arrives (previously only fetched once at startup)
  • HeartbeatInterval config (pkg/vmcp/server/server.go, serve.go, derive.go): Make SSE keep-alive interval configurable for testing (defaults to 30s in production, 100ms in tests)
  • WithSessionTTL helper (test/integration/vmcp/helpers/vmcp_server.go): Allow integration tests to configure short session TTLs

Real regressions found and documented

  1. mcpcompat shim drops progress/logging notifications and cannot forward list_changed downstream #5768 — Shim drops progress/logging notifications and cannot forward list_changed downstream
  2. mcpcompat shim advertises only {logging:{}} instead of capabilities.tools and capabilities.resources #5769 — Shim advertises only {logging:{}} instead of capabilities.tools and capabilities.resources (CRITICAL — breaks spec-compliant clients)
  3. mcpcompat shim normalizes tool schemas, dropping properties when type:object is absent #5770 — Shim normalizes tool schemas, dropping properties when type:object is absent
  4. vMCP backend queries do not follow pagination cursors (silent truncation past 1000 tools) #5771 — vMCP backend queries do not follow pagination cursors (silent truncation past 1000 tools)

Verification

task build  → EXIT=0
task test   → EXIT=0 (all tests pass, 2 skipped with documented reasons)
task lint   → 0 issues

Notes

  • All imports use github.com/stacklok/toolhive-core/mcpcompat/* — no direct mark3labs/mcp-go imports
  • Tests use raw JSON-RPC HTTP (encoding/json) and toolhive's own types (pkg/vmcp) where possible to minimize SDK coupling
  • Skipped tests serve as regression anchors — they should be un-skipped when the corresponding shim fixes land in toolhive-core#156

Related

JAORMX added 4 commits July 9, 2026 15:38
Pin current mcpcompat shim behavior for session lifecycle,
capability advertisement, tool schema fidelity, and concurrent
context isolation so the go-sdk migration can be validated.

Fix bridge to re-fetch tools/resources/prompts when a list_changed
notification arrives, keeping the local stdio server's advertised set
in sync with the upstream backend.

Two real regressions found and pinned with documented assertions:
- Initialize response advertises only {logging:{}} instead of
  capabilities.tools and capabilities.resources (toolhive-core#156 U1)
- normalizeObjectSchema replaces schemas without type:object with
  a bare {type:object}, dropping properties and required fields
  (toolhive-core#156 U6)

Refs #5742
Pin current mcpcompat shim behavior for auth error classification
and local proxy Host-header rewriting. Add configurable
HeartbeatInterval to ServerConfig for testable SSE keep-alive.

Tests cover 401 to ErrAuthenticationFailed, 403-on-initialize to
legacy SSE fallback, tool errors with auth-looking messages not
being misclassified as auth failures, and Host-header rewriting in
the transparent proxy.

Key finding: the streamable-HTTP transport does not return
transport.ErrLegacySSEServer for 403 responses (only the SSE
transport does), so the sentinel is not in the error chain.  The
error is still classified as ErrBackendUnavailable via string
matching.  Pinned with documented assertions.

Refs #5742
Pin current mcpcompat shim behavior for SSE keep-alive cadence,
tool pagination, per-session tool projection with Cedar authz,
and sliding session TTL so the go-sdk migration can be validated.

Tests cover keep-alive bytes on idle SSE streams, >1000 tool
listing behavior (skipped — known pagination gap), two sessions
with different authz outcomes seeing different tools, filtered
tools being uncallable, session tool sets being fixed at
initialize, and active sessions not expiring despite TTL elapsing.

Add WithSessionTTL helper option for integration tests.

Refs #5742
Pin the current mcpcompat shim behavior for the stdio bridge's
list_changed re-fetch fix and document known shim limitations.

Test covers the forwardAll re-sync when a tools/list_changed
notification arrives from the upstream backend, verifying the
bridge's local server picks up the updated tool set.

Skipped test documents that progress and logging notifications are
dropped by the mcpcompat client shim (no ProgressNotificationHandler
or LoggingMessageHandler installed) and that list_changed
notifications cannot be forwarded downstream (SendNotificationToAllClients
only handles progress and message).  Serves as regression anchor
for when the shim is fixed.

Refs #5742
@github-actions github-actions Bot added the size/XL Extra large PR: 1000+ lines changed label Jul 9, 2026

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Large PR Detected

This PR exceeds 1000 lines of changes and requires justification before it can be reviewed.

How to unblock this PR:

Add a section to your PR description with the following format:

## Large PR Justification

[Explain why this PR must be large, such as:]
- Generated code that cannot be split
- Large refactoring that must be atomic
- Multiple related changes that would break if separated
- Migration or data transformation

Alternative:

Consider splitting this PR into smaller, focused changes (< 1000 lines each) for easier review and reduced risk.

See our Contributing Guidelines for more details.


This review will be automatically dismissed once you add the justification section.

@codecov

codecov Bot commented Jul 9, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 93.33333% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 70.84%. Comparing base (3156cb9) to head (bc3d074).

Files with missing lines Patch % Lines
pkg/vmcp/server/server.go 80.00% 1 Missing ⚠️
Additional details and impacted files
@@                  Coverage Diff                   @@
##           feat/remove-mcp-go    #5772      +/-   ##
======================================================
+ Coverage               70.77%   70.84%   +0.07%     
======================================================
  Files                     683      683              
  Lines                   69194    69208      +14     
======================================================
+ Hits                    48970    49030      +60     
+ Misses                  16625    16591      -34     
+ Partials                 3599     3587      -12     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@github-actions

Copy link
Copy Markdown
Contributor

✅ Large PR justification has been provided. The size review has been dismissed and this PR can now proceed with normal review.

@github-actions
github-actions Bot dismissed their stale review July 13, 2026 06:52

Large PR justification has been provided. Thank you!

@github-actions github-actions Bot added the size/XL Extra large PR: 1000+ lines changed label Jul 13, 2026
Codespell flagged rIn as a potential misspelling (ring/rink/rain).
Rename to pipeR/pipeW to avoid the false positive.

Refs #5742
@github-actions github-actions Bot added size/XL Extra large PR: 1000+ lines changed and removed size/XL Extra large PR: 1000+ lines changed labels Jul 13, 2026
@rdimitrov
rdimitrov merged commit fc5466d into feat/remove-mcp-go Jul 13, 2026
38 of 39 checks passed
@rdimitrov
rdimitrov deleted the test/regression-mcp-go-migration branch July 13, 2026 07:56
JAORMX added a commit that referenced this pull request Jul 16, 2026
* Add Serve-path regression tests and bridge list_changed re-fetch

Pin current mcpcompat shim behavior for session lifecycle,
capability advertisement, tool schema fidelity, and concurrent
context isolation so the go-sdk migration can be validated.

Fix bridge to re-fetch tools/resources/prompts when a list_changed
notification arrives, keeping the local stdio server's advertised set
in sync with the upstream backend.

Two real regressions found and pinned with documented assertions:
- Initialize response advertises only {logging:{}} instead of
  capabilities.tools and capabilities.resources (toolhive-core#156 U1)
- normalizeObjectSchema replaces schemas without type:object with
  a bare {type:object}, dropping properties and required fields
  (toolhive-core#156 U6)

Refs #5742

* Add auth-error mapping, Host-header, and HeartbeatInterval config

Pin current mcpcompat shim behavior for auth error classification
and local proxy Host-header rewriting. Add configurable
HeartbeatInterval to ServerConfig for testable SSE keep-alive.

Tests cover 401 to ErrAuthenticationFailed, 403-on-initialize to
legacy SSE fallback, tool errors with auth-looking messages not
being misclassified as auth failures, and Host-header rewriting in
the transparent proxy.

Key finding: the streamable-HTTP transport does not return
transport.ErrLegacySSEServer for 403 responses (only the SSE
transport does), so the sentinel is not in the error chain.  The
error is still classified as ErrBackendUnavailable via string
matching.  Pinned with documented assertions.

Refs #5742

* Add SSE keep-alive, pagination, projection, and sliding TTL tests

Pin current mcpcompat shim behavior for SSE keep-alive cadence,
tool pagination, per-session tool projection with Cedar authz,
and sliding session TTL so the go-sdk migration can be validated.

Tests cover keep-alive bytes on idle SSE streams, >1000 tool
listing behavior (skipped — known pagination gap), two sessions
with different authz outcomes seeing different tools, filtered
tools being uncallable, session tool sets being fixed at
initialize, and active sessions not expiring despite TTL elapsing.

Add WithSessionTTL helper option for integration tests.

Refs #5742

* Add stdio bridge notification forwarding regression tests

Pin the current mcpcompat shim behavior for the stdio bridge's
list_changed re-fetch fix and document known shim limitations.

Test covers the forwardAll re-sync when a tools/list_changed
notification arrives from the upstream backend, verifying the
bridge's local server picks up the updated tool set.

Skipped test documents that progress and logging notifications are
dropped by the mcpcompat client shim (no ProgressNotificationHandler
or LoggingMessageHandler installed) and that list_changed
notifications cannot be forwarded downstream (SendNotificationToAllClients
only handles progress and message).  Serves as regression anchor
for when the shim is fixed.

Refs #5742

* Fix codespell: rename rIn/wIn to pipeR/pipeW in bridge test

Codespell flagged rIn as a potential misspelling (ring/rink/rain).
Rename to pipeR/pipeW to avoid the false positive.

Refs #5742
JAORMX added a commit that referenced this pull request Jul 16, 2026
Rebasing onto main advanced toolhive-core to a release where three
mcpcompat behaviors the #5772 gating tests pinned have changed:

- Schema fidelity: a tool schema without a top-level "type":"object"
  is now projected intact (properties/required preserved, type
  supplied) rather than normalized to the empty object schema.
- initialize capabilities: tools and resources are now advertised in
  the initialize response alongside logging.
- SSE keep-alive: an idle GET stream now emits periodic keep-alive
  comments; the test asserts the configured heartbeat by passing
  WithHeartbeatInterval to the streamable server it builds.

Flip the first two per their own documented instructions and wire the
heartbeat interval into the keep-alive test so it asserts the real
keep-alive rather than an incidental notification.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UthZzKery5m7GBNcXYKALe
JAORMX added a commit that referenced this pull request Jul 16, 2026
* Add Serve-path regression tests and bridge list_changed re-fetch

Pin current mcpcompat shim behavior for session lifecycle,
capability advertisement, tool schema fidelity, and concurrent
context isolation so the go-sdk migration can be validated.

Fix bridge to re-fetch tools/resources/prompts when a list_changed
notification arrives, keeping the local stdio server's advertised set
in sync with the upstream backend.

Two real regressions found and pinned with documented assertions:
- Initialize response advertises only {logging:{}} instead of
  capabilities.tools and capabilities.resources (toolhive-core#156 U1)
- normalizeObjectSchema replaces schemas without type:object with
  a bare {type:object}, dropping properties and required fields
  (toolhive-core#156 U6)

Refs #5742

* Add auth-error mapping, Host-header, and HeartbeatInterval config

Pin current mcpcompat shim behavior for auth error classification
and local proxy Host-header rewriting. Add configurable
HeartbeatInterval to ServerConfig for testable SSE keep-alive.

Tests cover 401 to ErrAuthenticationFailed, 403-on-initialize to
legacy SSE fallback, tool errors with auth-looking messages not
being misclassified as auth failures, and Host-header rewriting in
the transparent proxy.

Key finding: the streamable-HTTP transport does not return
transport.ErrLegacySSEServer for 403 responses (only the SSE
transport does), so the sentinel is not in the error chain.  The
error is still classified as ErrBackendUnavailable via string
matching.  Pinned with documented assertions.

Refs #5742

* Add SSE keep-alive, pagination, projection, and sliding TTL tests

Pin current mcpcompat shim behavior for SSE keep-alive cadence,
tool pagination, per-session tool projection with Cedar authz,
and sliding session TTL so the go-sdk migration can be validated.

Tests cover keep-alive bytes on idle SSE streams, >1000 tool
listing behavior (skipped — known pagination gap), two sessions
with different authz outcomes seeing different tools, filtered
tools being uncallable, session tool sets being fixed at
initialize, and active sessions not expiring despite TTL elapsing.

Add WithSessionTTL helper option for integration tests.

Refs #5742

* Add stdio bridge notification forwarding regression tests

Pin the current mcpcompat shim behavior for the stdio bridge's
list_changed re-fetch fix and document known shim limitations.

Test covers the forwardAll re-sync when a tools/list_changed
notification arrives from the upstream backend, verifying the
bridge's local server picks up the updated tool set.

Skipped test documents that progress and logging notifications are
dropped by the mcpcompat client shim (no ProgressNotificationHandler
or LoggingMessageHandler installed) and that list_changed
notifications cannot be forwarded downstream (SendNotificationToAllClients
only handles progress and message).  Serves as regression anchor
for when the shim is fixed.

Refs #5742

* Fix codespell: rename rIn/wIn to pipeR/pipeW in bridge test

Codespell flagged rIn as a potential misspelling (ring/rink/rain).
Rename to pipeR/pipeW to avoid the false positive.

Refs #5742
JAORMX added a commit that referenced this pull request Jul 16, 2026
Rebasing onto main advanced toolhive-core to a release where three
mcpcompat behaviors the #5772 gating tests pinned have changed:

- Schema fidelity: a tool schema without a top-level "type":"object"
  is now projected intact (properties/required preserved, type
  supplied) rather than normalized to the empty object schema.
- initialize capabilities: tools and resources are now advertised in
  the initialize response alongside logging.
- SSE keep-alive: an idle GET stream now emits periodic keep-alive
  comments; the test asserts the configured heartbeat by passing
  WithHeartbeatInterval to the streamable server it builds.

Flip the first two per their own documented instructions and wire the
heartbeat interval into the keep-alive test so it asserts the real
keep-alive rather than an incidental notification.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UthZzKery5m7GBNcXYKALe
JAORMX added a commit that referenced this pull request Jul 17, 2026
…k) (#5729)

* feat: migrate from mark3labs/mcp-go to toolhive-core/mcpcompat

Swaps all github.com/mark3labs/mcp-go/{mcp,server,client,client/transport}
imports to the go-sdk-backed compatibility shim in toolhive-core
(github.com/stacklok/toolhive-core/mcpcompat/*). No call-site logic changes —
the shim mirrors mcp-go's API. This moves ToolHive's MCP protocol handling
onto the official modelcontextprotocol/go-sdk.

Depends on stacklok/toolhive-core#150; the toolhive-core dependency is pinned
to that branch commit and should be bumped to the released version once #150
merges.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

* chore: bump toolhive-core shim + fix gci import grouping

Bump github.com/stacklok/toolhive-core to the mcpcompat shim commit that
closes the server/client compatibility gaps exercised by the vMCP and MCP
test suites (pulls the required cel-go v0.29.1). Also fix gci import
grouping in three files flagged by lint (toolhive-core shares the
github.com/stacklok/toolhive prefix, so its imports group with toolhive's).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

* style: fix gci import grouping after mcp-go->mcpcompat swap

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

* Add regression tests gating the mcp-go to go-sdk migration (#5772)

* Add Serve-path regression tests and bridge list_changed re-fetch

Pin current mcpcompat shim behavior for session lifecycle,
capability advertisement, tool schema fidelity, and concurrent
context isolation so the go-sdk migration can be validated.

Fix bridge to re-fetch tools/resources/prompts when a list_changed
notification arrives, keeping the local stdio server's advertised set
in sync with the upstream backend.

Two real regressions found and pinned with documented assertions:
- Initialize response advertises only {logging:{}} instead of
  capabilities.tools and capabilities.resources (toolhive-core#156 U1)
- normalizeObjectSchema replaces schemas without type:object with
  a bare {type:object}, dropping properties and required fields
  (toolhive-core#156 U6)

Refs #5742

* Add auth-error mapping, Host-header, and HeartbeatInterval config

Pin current mcpcompat shim behavior for auth error classification
and local proxy Host-header rewriting. Add configurable
HeartbeatInterval to ServerConfig for testable SSE keep-alive.

Tests cover 401 to ErrAuthenticationFailed, 403-on-initialize to
legacy SSE fallback, tool errors with auth-looking messages not
being misclassified as auth failures, and Host-header rewriting in
the transparent proxy.

Key finding: the streamable-HTTP transport does not return
transport.ErrLegacySSEServer for 403 responses (only the SSE
transport does), so the sentinel is not in the error chain.  The
error is still classified as ErrBackendUnavailable via string
matching.  Pinned with documented assertions.

Refs #5742

* Add SSE keep-alive, pagination, projection, and sliding TTL tests

Pin current mcpcompat shim behavior for SSE keep-alive cadence,
tool pagination, per-session tool projection with Cedar authz,
and sliding session TTL so the go-sdk migration can be validated.

Tests cover keep-alive bytes on idle SSE streams, >1000 tool
listing behavior (skipped — known pagination gap), two sessions
with different authz outcomes seeing different tools, filtered
tools being uncallable, session tool sets being fixed at
initialize, and active sessions not expiring despite TTL elapsing.

Add WithSessionTTL helper option for integration tests.

Refs #5742

* Add stdio bridge notification forwarding regression tests

Pin the current mcpcompat shim behavior for the stdio bridge's
list_changed re-fetch fix and document known shim limitations.

Test covers the forwardAll re-sync when a tools/list_changed
notification arrives from the upstream backend, verifying the
bridge's local server picks up the updated tool set.

Skipped test documents that progress and logging notifications are
dropped by the mcpcompat client shim (no ProgressNotificationHandler
or LoggingMessageHandler installed) and that list_changed
notifications cannot be forwarded downstream (SendNotificationToAllClients
only handles progress and message).  Serves as regression anchor
for when the shim is fixed.

Refs #5742

* Fix codespell: rename rIn/wIn to pipeR/pipeW in bridge test

Codespell flagged rIn as a potential misspelling (ring/rink/rain).
Rename to pipeR/pipeW to avoid the false positive.

Refs #5742

* Update migration gating tests for new toolhive-core behavior

Rebasing onto main advanced toolhive-core to a release where three
mcpcompat behaviors the #5772 gating tests pinned have changed:

- Schema fidelity: a tool schema without a top-level "type":"object"
  is now projected intact (properties/required preserved, type
  supplied) rather than normalized to the empty object schema.
- initialize capabilities: tools and resources are now advertised in
  the initialize response alongside logging.
- SSE keep-alive: an idle GET stream now emits periodic keep-alive
  comments; the test asserts the configured heartbeat by passing
  WithHeartbeatInterval to the streamable server it builds.

Flip the first two per their own documented instructions and wire the
heartbeat interval into the keep-alive test so it asserts the real
keep-alive rather than an incidental notification.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UthZzKery5m7GBNcXYKALe

* Bump toolhive-core to v0.0.29

Pick up the CallGate seam and the passive SSE keep-alive so the
mcpcompat migration includes the keep-alive fix the gating test asserts.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UthZzKery5m7GBNcXYKALe

* Reject a terminated vMCP session with 404, not 503

Under the mcpcompat transport, a request for a session that was
terminated on another replica came back HTTP 503 instead of 404, so the
client retried the dead session forever instead of re-initializing —
a cross-replica lazy-eviction regression surfaced by the SDK migration
(mcp-go answered a blanket 404 on any validation failure).

Manager.Terminate deletes the storage key (unchanged — deliberate, to
avoid the Phase 1->2 resurrection race). The bug was that Manager.Validate
then reported the absent key as (isTerminated=false, error), which the
transport maps to a retryable 503. A genuinely-gone session (deleted,
TTL-expired, or never existed) is not transient: report it as
(isTerminated=true, nil) so the transport answers a definitive 404 and
the client re-initializes. A real, transient storage error still returns
(false, error) -> 503, so a store blip does not drop live session state.

Terminate is untouched, so the resurrection-race guarantee is preserved;
only the way an absent key is reported by Validate changes.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UthZzKery5m7GBNcXYKALe

* Verify shim forwards progress/logging notifications

Replace a stale skipped regression anchor that claimed the mcpcompat
client dropped upstream progress/logging notifications. The client wires
both the go-sdk ProgressNotificationHandler and LoggingMessageHandler
(toolhive-core issue #156), and the bridge relays every notification
downstream, so the premise no longer holds.

Add a real test driving a live backend: notifications/progress forwards
to the client's OnNotification handler unconditionally, and
notifications/message forwards once the client subscribes via
SetLoggingLevel (level-gated per the MCP spec, matching mcp-go). The
test fails if either handler regresses.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UthZzKery5m7GBNcXYKALe

* Address review: strengthen migration tests, fix stale comments

Reviewer feedback on the mcp-go -> mcpcompat migration:

Test quality (false-green gates):
- sse_keepalive: serve srv.Handler(ctx) through httptest so the
  configured interval flows Config -> Handler -> WithHeartbeatInterval;
  the prior form built its own server and never gated that wiring.
- sliding_ttl: active half drives tools/call (through
  enforceSessionBinding -> storage.Load) instead of tools/list, which
  returns the cached set even after eviction and couldn't distinguish a
  sliding TTL from a fixed one.
- host_header: assert tracingTransport.RoundTrip directly with a request
  whose Host differs from URL.Host, so the test fails if the Host-rewrite
  is removed.
- context_isolation: collect goroutine results and assert after
  wg.Wait() (require/FailNow is unsafe off the test goroutine); rename the
  second block to what it actually asserts.

Coverage:
- auth_error_mapping: pin wrapBackendError's 5xx, timeout, EOF/refused,
  and transport.ErrAuthorizationRequired -> ErrAuthenticationFailed
  branches (the migration-sensitive sentinel).

Hygiene:
- bridge.go: soften the forwardAll comment (additive-only; does not prune
  capabilities removed upstream).
- server.go: heartbeat doc says "zero or negative" (matches the d<=0
  guard); note HeartbeatInterval is plumbed ahead of CLI/CRD wiring.
- Update stale "mark3labs" doc comments to "mcpcompat".

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UthZzKery5m7GBNcXYKALe

---------

Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/XL Extra large PR: 1000+ lines changed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants